Reject ParamSpec-typed callables calls with insufficient arguments #17323

sterliakov · 2024-06-04T01:27:06Z

When type checking a call of a ParamSpec-typed callable, currently there is an incorrect "fast path" (if there are two arguments of shape (*args: P.args, **kwargs: P.kwargs), accept), which breaks with Concatenate (such call was accepted even for Concatenate[int, P]).

Also there was no checking that args and kwargs are actually present: since *args and **kwargs are not required, their absence was silently accepted.

sterliakov · 2024-06-04T01:50:38Z

Is the terrific Windows failure at test teardown really caused by my changes?..

JelleZijlstra · 2024-06-04T01:56:25Z

Is the terrific Windows failure at test teardown really caused by my changes?..

Probably not, I think I've seen that one before.

Separately, thanks for the PR! It would be good if you could go through the new errors in mypy-primer to check whether they are correct or false positives.

sterliakov · 2024-06-04T02:04:55Z

There is one severe false positive, caused by overload. I have a test case for it now and will try to resolve this problem tomorrow, then go through the rest. New errors in trio and scrapy are caused by that.

[case testRunParamSpecOverload]
from typing_extensions import ParamSpec, Concatenate
from typing import Callable, overload, NoReturn, TypeVar, Union

P = ParamSpec("P")
T = TypeVar("T")

@overload
def capture(
    sync_fn: Callable[P, NoReturn],
    *args: P.args,
    **kwargs: P.kwargs,
) -> int: ...
@overload
def capture(
    sync_fn: Callable[P, T],
    *args: P.args,
    **kwargs: P.kwargs,
) -> Union[T, int]: ...
def capture(
    sync_fn: Callable[P, T],
    *args: P.args,
    **kwargs: P.kwargs,
) -> Union[T, int]:
    return sync_fn(*args, **kwargs)

def fn() -> int: return 1
capture(fn)

[builtins fixtures/paramspec.pyi]

sterliakov · 2024-06-04T14:14:15Z

Hm, and this primer diff looks marginally better.

starlette: the only false positive bit here, as far as I understand. How critical is partial+ParamSpec support? It is fundamentally different from any other ParamSpec-typed calls, because early binding allows omission of *args and/or **kwargs ParamSpec parts, but only if they were provided to partial, plus *args cannot be provided twice while **kwargs can. I have just crafted a patch to support all variations of such binding, but it involves adding one new attribute to CallableType, and I'm not sure whether such change is worth the performance impact. I can submit that change as a separate follow-up PR if you agree that this case is not critical.
```
from functools import partial
from typing_extensions import ParamSpec
from typing import Callable, TypeVar

P = ParamSpec("P")
T = TypeVar("T")

def run(func: Callable[P, T], *args: P.args, **kwargs: P.kwargs) -> T:
    func2 = partial(func, *args, **kwargs)
    return func2()

def run2(func: Callable[P, T], *args: P.args, **kwargs: P.kwargs) -> T:
    func2 = partial(func, *args)
    return func2(**kwargs)
```
optuna: I'd call this a true positive, because fn: Callable[P, T]; kwargs: Any, fn(**kwargs) is obviously unsafe and fails for a function with posonly arguments. In fact such case is validated before and ends with early TypeError, but I don't believe this can be explained to mypy.
pandas-stubs: all errors are the same, correspond to lines where there's already a type: ignore comment followed by wider pyright: ignore[reportArgumentType,reportCallIssue] comment. Now mypy is closer to correct behaviour IMO. In some cases good exact argument error became a less precise "no overload variant...", but it's a fair price for actually checking that no args are missing. I don't see how to overcome that (yet). Caused by excessive strictness of ExpressionChecker.erased_signature_similarity.
pwndbg: The error is true positive (!), this looks like a bug in their code: there is a func: Callable[P, T], which is then called as func(). I don't know what note: At top level: means.

TeamSpen210 · 2024-06-04T23:02:37Z

For the starlette error, I do know anyio.to_thread.run_sync() will ultimately just pass the args to the func, so altering the code to bind both via partial would behave identically and is probably more correct anyway.

sterliakov · 2024-06-10T01:10:38Z

I have cleaned up code here a little bit, this should be now ready for review. #17355 is the follow-up patch I promised for functools.partial based on this (marked draft because it contains all code from this branch and I'd probably want them reviewed separately).

sterliakov · 2024-06-24T13:01:40Z

Cc @JelleZijlstra in a hope for review here:) This PR is a prerequisite for #17355 which overlaps with @ilevkivskyi ongoing ParamSpec work. I'm also fine with abandoning this and doing everything there in single PR, please let me know what granularity looks better for you.

A5rocks · 2024-08-21T03:51:24Z

mypy/checkexpr.py

+                if typ.is_generic() and typ.param_spec() is not None:
+                    typ, formal_to_actual = self.adjust_generic_callable_params_mapping(
+                        typ, args, arg_kinds, arg_names, formal_to_actual, context
+                    )


I'm confused about why this depends on typ.param_spec() when it doesn't in check_callable_call. Removing that makes sense to me but also fails a ton of test cases. Do you know why?

Just noticed you added this in a commit presumably after seeing those test case failures. Do you have any sort of deeper intuition for why this is right? (this is the only bit that doesn't make sense to me)

Plain typevars are irrelevant here: this function looks for targets that have proper arguments count (signature shape, ignoring parameters types). Typevar resolution cannot affect this shape since it's a 1:1 mapping. ParamSpec, on the other hand, might be expanded into some fixed-arg form if we can solve this ParamSpec now.

Removing this constraint results in a bunch of overload resolution failures, like

from typing import Any, Callable, overload, TypeVar F = TypeVar('F', bound=Callable[..., Any]) @overload def dec(x: F) -> F: ... @overload def dec(x: str) -> Callable[[F], F]: ... def dec(x) -> Any: pass @dec def f(name: str) -> int: return 0 @dec('abc') def g(name: str) -> int: return 0 reveal_type(f) # N: Revealed type is "def (name: builtins.str) -> builtins.int" reveal_type(g) # N: Revealed type is "def (name: builtins.str) -> builtins.int"

If we try to expand, dec("abc") begins to match the first overload, and then "str not callable". That's because infer_function_type_arguments is too lax and does not include some constraints - I don't know whether it's intended, but sounds difficult to fix. Such impossible resolutions are filtered out later in check_callable_call, so it doesn't cause any false negatives with regular calls - but plausible_overload_call_targets does not need such precision, we're just filtering out overloads that certainly can't match.

This piece supports scenarios similar to testRunParamSpecOverload testcase - without such check, ParamSpec overloads will be unexpectedly filtered out. As an alternative, we can avoid this expansion entirely and just report a match whenever paramspec is encountered in overload - that may be even more robust, actually.

Hm, and that alternative also passes all testcases. I think it's a safer solution that won't harm: the most severe impact of adding an extra overload would be a performance overhead to check one. Overload+paramspec isn't a very popular combination, so the performance impact shouldn't be noticeable. I pushed a version with this update, let's see what CI says.

…ec-missing-args

sterliakov · 2024-08-21T14:02:37Z

Diff review:

Starlette: false positive on a weird case (partial + overload), resolved in Support ParamSpec mapping with functools.partial #17355
Optuna: true positive (fn(**Any) where fn(*args: P.args, **kwargs: P.kwargs))
Code (homeassistant): true positive (args not passed, plain fn())
pwndbg: true positive, but with spurious note added
pandas & pandas-stubs: slightly worse overload resolution (instead of saying "here's the closest one, but still a miss" now mypy says "not even close, please try again"). All errors were already there, only code and/or lineno changed.

A5rocks

This makes sense to me. I'm not really a maintainer but hopefully this review is a useful signal :P

One tiny code style thing that doesn't really matter.

A5rocks · 2024-08-21T14:20:09Z

mypy/checkexpr.py

@@ -2754,18 +2737,58 @@ def check_overload_call(
            self.chk.fail(message_registry.TOO_MANY_UNION_COMBINATIONS, context)
        return result

+    def adjust_generic_callable_params_mapping(


This doesn't need to be deduplicated anymore because it's only called once

Honestly, I'd rather keep this separated: that block is rather long and not absolutely trivial, it looks better as a separate method to my eyes. However, there was another problem not detected by linter for some reason (uhm, sorry, because ARG001 is not enabled?.. Why?..), pushed to remove arguments that are no longer necessary.

sterliakov · 2024-08-21T22:30:58Z

Wow, it's the same problem as in the first workflow run for this PR. I hardly know anything about Windows, but still doesn't seem to be caused by the changes here.

A5rocks · 2024-08-28T23:00:30Z

Wow, it's the same problem as in the first workflow run for this PR. I hardly know anything about Windows, but still doesn't seem to be caused by the changes here.

I also got it on a seperate PR. It's not caused by the changes here.

…parate PR

github-actions · 2024-09-24T06:24:27Z

Diff from mypy_primer, showing the effect of this PR on open source code:

pandas (https://github.com/pandas-dev/pandas)
+ pandas/core/missing.py:422: error: Unused "type: ignore" comment  [unused-ignore]
+ pandas/core/missing.py:422: error: No overload variant of "apply_along_axis" matches argument types "Callable[[ndarray[Any, Any]], None]", "int", "ndarray[Any, Any]"  [call-overload]
+ pandas/core/missing.py:422: note: Error code "call-overload" not covered by "type: ignore" comment
+ pandas/core/missing.py:422: note: Possible overload variants:
+ pandas/core/missing.py:422: note:     def [_P`-1, _SCT: generic] apply_along_axis(func1d: Callable[[ndarray[Any, dtype[Any]], **_P], _SupportsArray[dtype[_SCT]] | _NestedSequence[_SupportsArray[dtype[_SCT]]]], axis: SupportsIndex, arr: _SupportsArray[dtype[Any]] | _NestedSequence[_SupportsArray[dtype[Any]]] | bool | int | float | complex | str | bytes | _NestedSequence[bool | int | float | complex | str | bytes], *args: _P.args, **kwargs: _P.kwargs) -> ndarray[Any, dtype[_SCT]]
+ pandas/core/missing.py:422: note:     def [_P`-1] apply_along_axis(func1d: Callable[[ndarray[Any, dtype[Any]], **_P], _SupportsArray[dtype[Any]] | _NestedSequence[_SupportsArray[dtype[Any]]] | bool | int | float | complex | str | bytes | _NestedSequence[bool | int | float | complex | str | bytes]], axis: SupportsIndex, arr: _SupportsArray[dtype[Any]] | _NestedSequence[_SupportsArray[dtype[Any]]] | bool | int | float | complex | str | bytes | _NestedSequence[bool | int | float | complex | str | bytes], *args: _P.args, **kwargs: _P.kwargs) -> ndarray[Any, dtype[Any]]

pwndbg (https://github.com/pwndbg/pwndbg)
+ pwndbg/gdblib/events.py: note: In function "connect":
+ pwndbg/gdblib/events.py:156: error: Too few arguments  [call-arg]

pandas-stubs (https://github.com/pandas-dev/pandas-stubs)
+ tests/test_resampler.py:237: error: Unused "type: ignore" comment  [unused-ignore]
+ tests/test_resampler.py:237: error: No overload variant of "pipe" of "BaseGroupBy" matches argument types "Callable[[DatetimeIndexResampler[DataFrame], int, list[float], str, NamedArg(tuple[int], 'kw')], DataFrame]", "int", "list[float]", "int", "tuple[int]"  [call-overload]
+ tests/test_resampler.py:237: note: Error code "call-overload" not covered by "type: ignore" comment
+ tests/test_resampler.py:237: note: Possible overload variants:
+ tests/test_resampler.py:237: note:     def [P`27163, T] pipe(self, func: Callable[[DatetimeIndexResampler[DataFrame], **P], T], *args: P.args, **kwargs: P.kwargs) -> T
+ tests/test_resampler.py:237: note:     def [T] pipe(self, func: tuple[Callable[..., T], str], *args: Any, **kwargs: Any) -> T
+ tests/test_resampler.py:241: error: Unused "type: ignore" comment  [unused-ignore]
+ tests/test_resampler.py:244: error: Unused "type: ignore" comment  [unused-ignore]
+ tests/test_resampler.py:244: error: No overload variant of "pipe" of "BaseGroupBy" matches argument types "Callable[[DatetimeIndexResampler[DataFrame], int, list[float], str, NamedArg(tuple[int], 'kw')], DataFrame]", "int", "list[float]", "int", "tuple[int]"  [call-overload]
+ tests/test_resampler.py:244: note: Error code "call-overload" not covered by "type: ignore" comment
+ tests/test_resampler.py:244: note: Possible overload variants:
+ tests/test_resampler.py:244: note:     def [P`27177, T] pipe(self, func: Callable[[DatetimeIndexResampler[DataFrame], **P], T], *args: P.args, **kwargs: P.kwargs) -> T
+ tests/test_resampler.py:244: note:     def [T] pipe(self, func: tuple[Callable[..., T], str], *args: Any, **kwargs: Any) -> T
+ tests/test_resampler.py:248: error: Unused "type: ignore" comment  [unused-ignore]
+ tests/test_series.py:3112: error: Unused "type: ignore" comment  [unused-ignore]
+ tests/test_series.py:3112: error: No overload variant of "pipe" of "NDFrame" matches argument types "Callable[[Series[Any], int, list[float], str, NamedArg(tuple[int, int], 'keyword_only')], Series[Any]]", "int", "list[float]", "int", "tuple[int, int]"  [call-overload]
+ tests/test_series.py:3112: note: Error code "call-overload" not covered by "type: ignore" comment
+ tests/test_series.py:3112: note: Possible overload variants:
+ tests/test_series.py:3112: note:     def [P`44288, T] pipe(self, func: Callable[[Series[int], **P], T], *args: P.args, **kwargs: P.kwargs) -> T
+ tests/test_series.py:3112: note:     def [T] pipe(self, func: tuple[Callable[..., T], str], *args: Any, **kwargs: Any) -> T
+ tests/test_series.py:3116: error: Unused "type: ignore" comment  [unused-ignore]
+ tests/test_series.py:3119: error: Unused "type: ignore" comment  [unused-ignore]
+ tests/test_series.py:3119: error: No overload variant of "pipe" of "NDFrame" matches argument types "Callable[[Series[Any], int, list[float], str, NamedArg(tuple[int, int], 'keyword_only')], Series[Any]]", "int", "list[float]", "int", "tuple[int, int]"  [call-overload]
+ tests/test_series.py:3119: note: Error code "call-overload" not covered by "type: ignore" comment
+ tests/test_series.py:3119: note: Possible overload variants:
+ tests/test_series.py:3119: note:     def [P`44300, T] pipe(self, func: Callable[[Series[int], **P], T], *args: P.args, **kwargs: P.kwargs) -> T
+ tests/test_series.py:3119: note:     def [T] pipe(self, func: tuple[Callable[..., T], str], *args: Any, **kwargs: Any) -> T
+ tests/test_series.py:3123: error: Unused "type: ignore" comment  [unused-ignore]
+ tests/test_frame.py:1745: error: Unused "type: ignore" comment  [unused-ignore]
+ tests/test_frame.py:1745: error: No overload variant of "pipe" of "NDFrame" matches argument types "Callable[[DataFrame, int, list[float], str, NamedArg(tuple[int, int], 'keyword_only')], DataFrame]", "int", "list[float]", "int", "tuple[int, int]"  [call-overload]
+ tests/test_frame.py:1745: note: Error code "call-overload" not covered by "type: ignore" comment
+ tests/test_frame.py:1745: note: Possible overload variants:
+ tests/test_frame.py:1745: note:     def [P`64786, T] pipe(self, func: Callable[[DataFrame, **P], T], *args: P.args, **kwargs: P.kwargs) -> T
+ tests/test_frame.py:1745: note:     def [T] pipe(self, func: tuple[Callable[..., T], str], *args: Any, **kwargs: Any) -> T
+ tests/test_frame.py:1749: error: Unused "type: ignore" comment  [unused-ignore]
+ tests/test_frame.py:1752: error: Unused "type: ignore" comment  [unused-ignore]
+ tests/test_frame.py:1752: error: No overload variant of "pipe" of "NDFrame" matches argument types "Callable[[DataFrame, int, list[float], str, NamedArg(tuple[int, int], 'keyword_only')], DataFrame]", "int", "list[float]", "int", "tuple[int, int]"  [call-overload]
+ tests/test_frame.py:1752: note: Error code "call-overload" not covered by "type: ignore" comment
+ tests/test_frame.py:1752: note: Possible overload variants:
+ tests/test_frame.py:1752: note:     def [P`64798, T] pipe(self, func: Callable[[DataFrame, **P], T], *args: P.args, **kwargs: P.kwargs) -> T
+ tests/test_frame.py:1752: note:     def [T] pipe(self, func: tuple[Callable[..., T], str], *args: Any, **kwargs: Any) -> T
+ tests/test_frame.py:1756: error: Unused "type: ignore" comment  [unused-ignore]

core (https://github.com/home-assistant/core)
+ homeassistant/components/lamarzocco/coordinator.py:116: error: Too few arguments  [call-arg]

optuna (https://github.com/optuna/optuna)
+ optuna/_convert_positional_args.py:83: error: Too few arguments  [call-arg]

starlette (https://github.com/encode/starlette)
+ starlette/concurrency.py:38: error: Too few arguments  [call-arg]

hauntsaninja · 2024-09-24T06:26:26Z

Thank you, this is a very nice bug fix

Follow-up for #17323, resolving a false positive discovered there. Closes #17960. This enables use of `functools.partial` to bind some `*args` or `**kwargs` on a callable typed with `ParamSpec`. --------- Co-authored-by: Shantanu Jain <[email protected]>

Reject ParamSpec-typed callables calls with insufficient arguments

5cdb753